Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Add option to autobind user's email on registration #51

Merged
merged 4 commits into from
Jul 2, 2020

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Jun 29, 2020

Adds an option, bind_new_user_emails_to_sydent, which uses Sydent's internal bind api to automatically bind email addresses of users immediately after they register.

This is quite enterprise-specific, but could be generally useful to multiple organizations. This aims to solve the problem of requiring users to verify their email twice when using the functionality of an identity server in a corporate deployment - where both the homeserver and identity server are controlled. It does with while eliminating the need for the account_threepid_delegates.email option, which historically has been a very complicated option to reason about.

This is implemented as an option in preparation for potentially mainlining the code. We've decided it should be mainlined in module form instead.

TODO:

  • unit tests

@anoadragon453 anoadragon453 force-pushed the anoa/sydent_internal_bind branch from cb7f92d to 2d09951 Compare June 29, 2020 13:52
@babolivier
Copy link
Contributor

For context, we don't plan to port it on mainline because it's using the internal bind API, which is a Sydent-specific feature rather than one from the IS API.

@anoadragon453
Copy link
Member Author

@babolivier This is why I featured sydent so prominently in the config option. Eventually we will need to port this functionality to mainline though. Perhaps it would be better done in a Synapse module (which would be out of scope for this PR)?

@babolivier
Copy link
Contributor

This is why I featured sydent so prominently in the config option

Yeah, I just wanted to keep a written record here of the exact reason why we were doing this :)

Perhaps it would be better done in a Synapse module (which would be out of scope for this PR)?

Yes I think it should definitely be in a module (e.g. with a post-registration hook).

@anoadragon453
Copy link
Member Author

anoadragon453 commented Jun 29, 2020

Ok. If we're not mainlining it in this state though (config option), then I'll just remove the config option.

@babolivier
Copy link
Contributor

babolivier commented Jun 29, 2020

I'd advocate towards leaving it in - I assume we'll end up with a module that potentially does a lot of stuff and we'll probably want to be able to fine-tune what we run in which setup.

@anoadragon453
Copy link
Member Author

anoadragon453 commented Jun 29, 2020

tbh I don't think the config option is necessary to remind us that we should allow configuration of a module later.

However, after thinking it over, since we need to provide an IS URL somewhere (and it'd be gross to hard-code that somewhere into the codebase) a configu option is probably the best place to put it, mainline or not.

@anoadragon453 anoadragon453 requested a review from babolivier June 30, 2020 10:39
@anoadragon453 anoadragon453 force-pushed the anoa/sydent_internal_bind branch from 28e802c to f8e3d1b Compare June 30, 2020 10:41
Copy link
Contributor

@babolivier babolivier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm otherwise

synapse/handlers/identity.py Outdated Show resolved Hide resolved
synapse/handlers/identity.py Show resolved Hide resolved
synapse/handlers/register.py Outdated Show resolved Hide resolved
@anoadragon453 anoadragon453 merged commit 21821c0 into dinsic Jul 2, 2020
anoadragon453 added a commit that referenced this pull request Oct 14, 2020
…API (#66)

#51 added an option that would automatically bind a user's threepid to a configured identity server after they had registered. Unfortunately, when you bind threepids, ideally you would store that mapping in the database so that later on you can remove those mappings when you deactivate an account.

We found that due the fact that we did not store these mappings, threepids were not unbound upon user account deactivation.

This PR fixes the issue by creating the mappings again, meaning they will again be removed upon account deactivation.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants